-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handling of empty array in SQL condition generation #12254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handling of empty array in SQL condition generation #12254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should probably be a functional test for this. That way we'll be sure it works with all database platforms.
c042729 to
5d285c8
Compare
5d285c8 to
53fd92b
Compare
53fd92b to
aba173a
Compare
aba173a to
23f2286
Compare
|
@mpdude please review |
|
Unfortunately, provided solution doesn't work in our use case, same error: |
Can you give me more details, like which database system you’re using and when this happened? |
|
I can confirm that it works here with MySQL, ORM v3.5.3. |
| $value = [$value]; | ||
| } | ||
|
|
||
| if (empty($value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might compare array with === [] here.
I cannot reproduce your issue and it's not directetly related to my fix. Maybe create another issue ? i have tested with https://github.com/ramsey/uuid-doctrine/tree/main?tab=readme-ov-file#innodb-optimised-binary-uuids---deprecated and i cannot reproduce it. I have tried <?php
declare(strict_types=1);
namespace Doctrine\Tests\ORM\Functional\Ticket;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\Tests\OrmFunctionalTestCase;
use function random_bytes;
class GH12254Test extends OrmFunctionalTestCase
{
protected function setUp(): void
{
if (Type::hasType('uuid_binary_ordered_time')) {
Type::overrideType('uuid_binary_ordered_time', UuidType::class);
} else {
Type::addType('uuid_binary_ordered_time', UuidType::class);
}
parent::setUp();
$this->setUpEntitySchema([
GH12254EntityB::class,
]);
$this->_em->getConnection()->getDatabasePlatform()->registerDoctrineTypeMapping('uuid_binary_ordered_time', 'binary');
}
public function testGetTypeForUuidBinaryOrderedTimeType(): void
{
$entity = new GH12254EntityB();
$entity->id = random_bytes(16);
$this->_em->persist($entity);
$this->_em->flush();
$result = $this->_em->getRepository(GH12254EntityB::class)->findBy(['id' => []]);
$resultNonEmpty = $this->_em->getRepository(GH12254EntityB::class)->findBy(['id' => [$entity->id]]);
$this->assertEmpty($result);
$this->assertCount(1, $resultNonEmpty);
}
}
/**
* @Entity()
*/
class GH12254EntityB
{
/**
* @Column(type="uuid_binary_ordered_time")
* @Id()
* @GeneratedValue(strategy="NONE")
* @var UUidType
*/
public $id = null;
}
class UUidType extends Type
{
public const NAME = 'uuid_binary_ordered_time';
public function getName(): string
{
return self::NAME;
}
public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
{
return $platform->getBinaryTypeDeclarationSQL(
[
'length' => 16,
'fixed' => true,
]
);
}
} |
|
It could be unrelated, I think it's working for you, because it's simple case, but Ramsey has |
|
Sorry for confusion, I retract all my comments, it looks like we had bugfix, which doesn't work anymore. But thanks for fixing things! ❤️ |
|
@elliotbruneel Thank you for working on this! Two remarks, probably a matter of personal preference:
If you're looking for a suggestion, you may want to check https://github.com/doctrine/orm/compare/2.20.x...mpdude:doctrine2:fix-12190-regression?expand=1. |
|
Just noticed that |
My first goal was indeed to keep original generated code. I have edited the logic as it's more clear |
| identifier: missingType.generics | ||
| count: 1 | ||
| path: src/Internal/HydrationCompleteHandler.php | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's that coming from? Maybe you need to rebase to make it go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's come from the update of phpstan to 2.1.23
mpdude
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for the functional aspects, thank you.
Regarding PHPStan, @greg0ire please decide whether it makes sense in this PR
|
@mpdude as long as it is in a separate commit, which is the case, I'm satisfied. |
|
Thanks @elliotbruneel ! |
|
@mpdude |
|
Not |
|
postgresql: # SELECT * FROM unnest(ARRAY[true, false]) AS t(col) WHERE col IN (1=0);
col
-----
f
(1 row)
# SELECT * FROM unnest(ARRAY[null, true, false]) AS t(col) WHERE col IN (null);
col
-----
(0 rows)
@mpdude oh I see now, the suggestion was not to have |


Fix a regression introduced in #12190:
$repository->findBy(['uuid' => []]should be a valid expression and return an empty result.Fixes #12245, with additional discussion at #12190 (comment).